Skip to content

fix(firestore): honor database-scoped resource selectors - #11461

Open
dajiaohuang wants to merge 1 commit into
firebase:mainfrom
dajiaohuang:codex/firestore-database-scoped-resources
Open

dajiaohuang wants to merge 1 commit into
firebase:mainfrom
dajiaohuang:codex/firestore-database-scoped-resources

Conversation

@dajiaohuang

Copy link
Copy Markdown

Fixes #11447.

Database-scoped Firestore selectors now honor the optional resource suffix:

  • firestore:<databaseId|target>:rules prepares and deploys only rules for that database.
  • firestore:<databaseId|target>:indexes prepares and deploys only indexes for that database.
  • firestore:<databaseId|target> continues to select both resources.
  • Unsupported resource suffixes fail instead of being silently ignored.

Global firestore:rules and firestore:indexes selectors remain resource-specific. Comma-separated global and database-scoped selectors combine their resource selections.

Validation:

  • Focused Firestore specs: 14 passing.
  • ESLint on the four changed files: 0 errors; 44 warnings.
  • The project TypeScript compile reports seven diagnostics in unchanged files (dataconnect-sql-shell.ts, frameworks/astro/utils.ts, gcp/cloudsql/connect.ts, and logger.ts); none reference the changed files. Full npm test was not run.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors and improves the Firestore configuration parsing and resource selection logic during deployment. It introduces support for database-scoped resource targets (e.g., deploying only rules or indexes for a specific database) and simplifies the preparation step by centralizing the target selection logic in getFirestoreConfig. Feedback points out an inconsistency in the single-database configuration path, where global and database-scoped resource selections are not merged, unlike in the multi-database path.

Comment thread src/firestore/fsConfig.ts
Comment on lines +107 to +109
const resources = selection.allDatabases
? selection.allResources
: selection.byName.get(databaseId) || new Set<FirestoreResource>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In the single database configuration path, the code does not combine global and database-scoped resource selections. If selection.allDatabases is true (e.g., due to a global selector like firestore:rules), it directly uses selection.allResources and completely ignores any database-scoped selectors defined in selection.byName.\n\nThis is inconsistent with the multi-database array path below, which correctly merges both global and database-scoped resource selections for each database. This discrepancy was not caught by the tests because fsConfig.spec.ts only tests the array configuration, and prepare.spec.ts stubs getFirestoreConfig to return an array.

      const resources = new Set(selection.allDatabases ? selection.allResources : []);\n      for (const r of selection.byName.get(databaseId) || []) {\n        resources.add(r);\n      }

@joehan

joehan commented Nov 6, 2026

Copy link
Copy Markdown
Member

/joe-review

@joehan
joehan self-requested a review November 6, 2026 18:05
Comment thread src/firestore/fsConfig.ts
(parts.length === 3 && !resource) ||
(resource && resource !== "rules" && resource !== "indexes")
) {
throw new FirebaseError(`Invalid Firestore deploy target: ${value}`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 [Error Remediation & Exit Status] Provide actionable remediation and { exit: 1 }

Rationale: Per firebase-tools CLI error standards, user input validation errors thrown with FirebaseError must specify a non-zero exit code ({ exit: 1 }) and provide actionable remediation explaining the accepted target format.

Suggested Fix:

Suggested change
throw new FirebaseError(`Invalid Firestore deploy target: ${value}`);
throw new FirebaseError(
`Invalid Firestore deploy target: "${value}". Expected "firestore", "firestore:<databaseId|target>", "firestore:<databaseId|target>:rules", "firestore:<databaseId|target>:indexes", "firestore:rules", or "firestore:indexes".`,
{ exit: 1 },
);

Comment thread src/firestore/fsConfig.ts
const resources = selection.allDatabases
? selection.allResources
: selection.byName.get(databaseId) || new Set<FirestoreResource>();
if (!selection.allDatabases && !selection.byName.has(databaseId)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 [Error Handling & UX] Single-database configurations silently ignore non-matching database targets

Rationale: In multi-database mode (Array.isArray(fsConfig)), lines 159–166 throw a FirebaseError if selection.byName specifies a database target not found in firebase.json. However, in single-database mode (!Array.isArray(fsConfig)), lines 110–112 simply return [] when !selection.byName.has(databaseId).

When getFirestoreConfig returns [], src/deploy/firestore/prepare.ts exits cleanly, leading firebase deploy to report "Deploy complete!" while silently deploying nothing. Single-database mode should validate selection.byName and throw an informative FirebaseError with { exit: 1 } if any requested database target does not match databaseId.

Suggested Fix:

      if (!selection.allDatabases && !selection.byName.has(databaseId)) {
        const unmatched = [...selection.byName.keys()];
        throw new FirebaseError(
          `Could not find configurations in firebase.json for the following database targets: ${unmatched.join(", ")}`,
          { exit: 1 },
        );
      }

Comment thread src/firestore/fsConfig.ts
for (const resource of selection.byName.get(database) || []) {
resources.add(resource);
}
results.push(filterFirestoreConfig(c as ParsedFirestoreConfig, resources));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: Avoid as ParsedFirestoreConfig type assertion

Rationale: In the target branch above (line 141), { database, rules: c.rules, indexes: c.indexes } is constructed explicitly. In this database branch, using c as ParsedFirestoreConfig bypasses TypeScript compiler checks and preserves extra properties from c. Constructing the object explicitly is cleaner and avoids loose casting.

Suggested Fix:

Suggested change
results.push(filterFirestoreConfig(c as ParsedFirestoreConfig, resources));
results.push(
filterFirestoreConfig({ database, rules: c.rules, indexes: c.indexes }, resources),
);

Comment thread src/firestore/fsConfig.ts
const unmatched = [...selection.byName.keys()].filter(
(name) => !fsConfig.some((config) => config.database === name || config.target === name),
);
if (!selection.allDatabases && unmatched.length !== 0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: Specify { exit: 1 } on FirebaseError

Rationale: Per firebase-tools CLI standards, configuration and input assertion errors thrown with FirebaseError should explicitly specify { exit: 1 }.

Suggested Fix:

Suggested change
if (!selection.allDatabases && unmatched.length !== 0) {
throw new FirebaseError(
`Could not find configurations in firebase.json for the following database targets: ${[
...unmatched,
].join(", ")}`,
{ exit: 1 },
);

"Invalid Firestore deploy target: firestore:second:unknown",
);
});
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: Add unit test coverage for single-database configs and target aliases

Rationale: makeOptions in fsConfig.spec.ts currently sets firestore as an array (multi-DB). Adding test cases covering single-database configurations (e.g. firestore: { rules: "...", indexes: "..." } with --only firestore:rules, --only firestore:(default):rules, and unmatched database target rejection) will ensure full permutation coverage and prevent regressions across both configuration schemas.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi-DB: --only firestore:<databaseId> also deploys indexes, and firestore:<databaseId>:rules ignores the suffix

2 participants